Skip to content
This repository was archived by the owner on Sep 12, 2019. It is now read-only.

Don't allow child scripts to manipulate terminal #227

Merged
merged 5 commits into from
Jul 29, 2019

Conversation

RaeesBhatti
Copy link
Contributor

@RaeesBhatti RaeesBhatti commented Jul 24, 2019

- Summary
create-react-app script was using ANSI to clear the screen before we could display our own information. This change disables the handing over of terminal to child script, and buffering output from it. When create-react-app detects that it is not attached to a terminal, it does not clear screen.
Fixes: netlify/cli#374

- Test plan

  • npx create-react-app my-new-cra
  • cd my-new-cra
  • netlify dev

Before:
Screen Shot 2019-07-25 at 2 39 16 AM
After:
Screen Shot 2019-07-25 at 2 01 54 AM

- Description for the changelog

  • Dont handover terminal to child scripts
  • Buffer stdout and stderr from child scripts to terminal

- A picture of a cute animal (not mandatory but encouraged)
baby-seal-29vsgyf

@DavidWells
Copy link
Contributor

Looks like this removes colors from the wrapper

image

After

image

I'm wondering if this is possible while preserving child process colors... (DX etc)

@RaeesBhatti
Copy link
Contributor Author

RaeesBhatti commented Jul 25, 2019

Since we're not connecting the stdout and stderr of our terminal to child process, create-react-app script thinks that the terminal doesn't support colors, so, it is not sending us colored output at all. While I think what you're describing is technically possible, it is incredibly tricky. NodeJS gives us three options when it comes to child-process stdio:

  • inherit - what we were doing before
  • pipe - what we're doing now
  • ignore - you get the idea

We essentially have to create a terminal program that has a virtual terminal and manage that virtual terminal when resizing and flexing.

@swyxio
Copy link
Contributor

swyxio commented Jul 25, 2019

how bout goinginto the code of strip-ansi.. and ONLY removing whatever the "clear window" characters are

@DavidWells
Copy link
Contributor

@swyxio
Copy link
Contributor

swyxio commented Jul 25, 2019

well sheet is it that easy

@RaeesBhatti
Copy link
Contributor Author

RaeesBhatti commented Jul 25, 2019

Even if I don't call strip-ansi on the output, create-react-app still outputs un-colored text. Because we don't provide it a terminal, we provide it a buffer to write into. This is happening at chalk level, and can be tested by:

  • npm i -g chalk-cli
  • chalk red bold 'Unicorns & Rainbows' (will output colored text)
  • chalk red bold 'Unicorns & Rainbows' > something.txt && cat something.txt (will output uncolored text)

To have any kind of control over the output means that chalk won't work.

@RaeesBhatti
Copy link
Contributor Author

I've completely removed strip-ansi part in the latest change and the output is still uncolored. The reason is described above.

@RaeesBhatti
Copy link
Contributor Author

Found a way to fix this, but seems like I've lost write access to this repo.

@RaeesBhatti RaeesBhatti force-pushed the raees/terminal-clear-fix branch from 037bd17 to 24908b9 Compare July 29, 2019 12:21
@RaeesBhatti
Copy link
Contributor Author

With the latest change, the chalk colors and formatting is persevered.
Before latest change:
Screen Shot 2019-07-29 at 5 22 10 PM
After latest change:
Screen Shot 2019-07-29 at 5 22 50 PM

@jschatz1 jschatz1 merged commit 7633428 into master Jul 29, 2019
@jschatz1 jschatz1 deleted the raees/terminal-clear-fix branch July 29, 2019 18:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent terminal from being cleared in create-react-app project
4 participants